Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APP-6932 better local redirect validation #390

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DTCurrie
Copy link
Member

@DTCurrie DTCurrie commented Nov 12, 2024

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

@DTCurrie DTCurrie added the safe to test Mark as safe to test label Nov 12, 2024
@DTCurrie DTCurrie self-assigned this Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 12, 2024
@ohEmily
Copy link
Member

ohEmily commented Nov 12, 2024

trying to get a grip on this.

[q1] could you remind me why we have both a backto as well as a redirect_uri?

[q2] why can't we go with his proposed fix (which Google also confirmed is the best fix for an "open redirect attack"), which is Only allow fully qualified URLs from a whitelist?

Seems like FusionAuth would allow us to lock down our list of redirect URIs, hence q1 above:
Screenshot 2024-11-12 at 5 18 50 PM

@DTCurrie
Copy link
Member Author

trying to get a grip on this.

[q1] could you remind me why we have both a backto as well as a redirect_uri?

[q2] why can't we go with his proposed fix (which Google also confirmed is the best fix for an "open redirect attack"), which is Only allow fully qualified URLs from a whitelist?

Seems like FusionAuth would allow us to lock down our list of redirect URIs, hence q1 above: Screenshot 2024-11-12 at 5 18 50 PM

The redirect_uri is the path we are redirecting the users to from fusion auth into the app, the backto parameter is something we set in order to redirect the user back to the correct route in app after they have logged in. The flow would go something like:

  1. Logged out user goes to /registry
  2. They click the login button, which redirects them to /login with the current route set as the backto parameter
  3. In /login we check backto to see if it is valid, if so we save it in the session data
  4. They log in, and they are sent back to the redirect_uri, in this case /callback.
  5. In /callback we check the session data for a backto, in this case that would be /registry so we redirect them there instead of /

backto is unrelated to the redirect_uri and something we handle internally. I don't think fusion auth has a handler for this, if it does it would probably be a larger code change right now to use that over the current sessions implementation.

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
@DTCurrie DTCurrie removed the request for review from ohEmily November 14, 2024 14:47
@viambot viambot removed the safe to test Mark as safe to test label Nov 14, 2024
@viambot viambot added the safe to test Mark as safe to test label Nov 14, 2024
@DTCurrie DTCurrie requested a review from jr22 November 14, 2024 17:24
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Nov 14, 2024
Comment on lines +10 to +11
"viam.dev": true,
"viam.com": true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be app.viam.dev and app.viam.com?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left them as this because we have toyed with allowing users to log in from the marketing page and docs, but maybe I am being a little too forward-thinking here 😃 .

Let's start stricter; I'll update it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha! yeah i wasnt sure if it would pass the hostnameWhitelist[hostname] check for app.viam.com urls otherwise. could be misreading though

@jr22
Copy link
Member

jr22 commented Nov 14, 2024

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

is there an app pr up yet with these changes and the goutils bump so we can test?

@DTCurrie
Copy link
Member Author

Updates our backto redirect logic to only allow fully-qualified whitelisted https URLs. This will require a change in app to no longer pass just the current pathname for backto and instead to pass the full current URL.

is there an app pr up yet with these changes and the goutils bump so we can test?

Not yet, I will set that up.

@DTCurrie
Copy link
Member Author

Potential missing cases:

  • CI/Test envs
  • PR temp envs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Mark as safe to test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants